Skip to content

Conversation

@dustymabe
Copy link
Member

No description provided.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request aims to improve security by no longer running the COSA container as root. This is a great improvement. However, by removing the root privileges, the mechanism for using the newly built coreos-installer binary was removed but not replaced. My review includes a critical suggestion to fix this by updating the PATH environment variable, ensuring that the CI continues to test the correct binary.

dustymabe added a commit to dustymabe/coreos-assembler that referenced this pull request Jan 9, 2026
Allow group write permissions on /usr/bin because in upstream
project's CI we want to overwrite binaries for testing. The dir is
owned by root:root and CI runs in openshift as a user that is a
member of the `root` (GID: 0) group.

See coreos/coreos-installer#1716
@dustymabe
Copy link
Member Author

Ok I think this should be ready to go.

requires https://github.com/coreos/coreos-assembler/pull/4410/changes (please review)

@dustymabe dustymabe force-pushed the dusty-ci-as-root branch 2 times, most recently from 697848f to 7994979 Compare January 9, 2026 15:02
@dustymabe dustymabe marked this pull request as ready for review January 9, 2026 20:41
dustymabe added a commit to dustymabe/coreos-assembler that referenced this pull request Jan 16, 2026
Allow group write permissions on /usr/ because in upstream project's
CI we want to overwrite software for testing. The directories
are typically owned by root:root and CI runs in openshift as a user
that is a member of the `root` (GID: 0) group.

See coreos/coreos-installer#1716
dustymabe added a commit to dustymabe/coreos-assembler that referenced this pull request Jan 16, 2026
Allow group write permissions on /usr/ because in upstream project's
CI we want to overwrite software for testing. The directories
are typically owned by root:root and CI runs in openshift as a user
that is a member of the `root` (GID: 0) group.
See coreos/coreos-installer#1716

Also add an exception for /etc/grub.d for OSTree upstream CI.
dustymabe added a commit to coreos/coreos-assembler that referenced this pull request Jan 16, 2026
Allow group write permissions on /usr/ because in upstream project's
CI we want to overwrite software for testing. The directories
are typically owned by root:root and CI runs in openshift as a user
that is a member of the `root` (GID: 0) group.
See coreos/coreos-installer#1716

Also add an exception for /etc/grub.d for OSTree upstream CI.
@dustymabe dustymabe added the skip-notes This PR does not need release notes label Jan 16, 2026
@dustymabe dustymabe force-pushed the dusty-ci-as-root branch 2 times, most recently from ded3cd3 to 30f7807 Compare January 16, 2026 20:18
@dustymabe dustymabe enabled auto-merge (rebase) January 16, 2026 20:18
@aaradhak
Copy link
Member

aaradhak commented Jan 16, 2026

minor: Typo in the first commit message

ci: drop buildroot param to cosaPod
This _becaome_ obsolete when buildPod was introduced in

This became obsolete when buildPod was introduced in
coreos/coreos-ci-lib@f2a82bd
We opened up the permissions when building the COSA container [1] so
this isn't necessary any longer with a few adjustments here.

[1] coreos/coreos-assembler#4410
This issue should have been fixed a long time ago. Let's drop this
old workaround.
@dustymabe
Copy link
Member Author

minor: Typo in the first commit message

ci: drop buildroot param to cosaPod
This _becaome_ obsolete when buildPod was introduced in

fixed!

Copy link
Member

@aaradhak aaradhak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three commits LGTM.

@dustymabe dustymabe merged commit 055bf09 into coreos:main Jan 16, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-notes This PR does not need release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants